refactor: extract shared request handling from server entry points#258
refactor: extract shared request handling from server entry points#258yunus25jmi1 wants to merge 6 commits intocloudflare:mainfrom
Conversation
commit: |
yunus25jmi1
force-pushed
the
refactor/shared-request-handling
branch
from
14ccb24 to
631e3fc
Compare
|
All checks have passed with all green. Kindly review the changes. @southpolesteve @elithrar |
yunus25jmi1
force-pushed
the
refactor/shared-request-handling
branch
2 times, most recently
from
2cb7c43 to
6624adc
Compare
|
@southpolesteve review the changes. Conflicts resolved. |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Extract shared request handling from server entry points
The direction of this PR is right — deduplicating the inline __-prefixed helpers from the generated code into importable modules is a meaningful improvement. Having a single canonical matchConfigPattern (single-pass tokenizer) instead of the chained .replace() variant is a real fix.
However, there are several issues that need to be addressed before this can merge.
Key Issues
-
Dead code in
request-pipeline.ts— Four of the seven exported functions (guardProtocolRelativeUrl,stripBasePath,normalizeTrailingSlash,processMiddlewareHeaders) are never imported or called anywhere in the codebase. The protocol-relative guard is still inlined inapp-dev-server.tsat line 1125. This new module ships dead code that increases the bundle without any deduplication benefit. Either wire up these functions at the call sites that have the inline versions, or remove them from this PR and add them when they're actually used. -
Behavioral difference in
validateImageUrl— The old code resolved the image URL againstrequest.url(full URL with path+query), while the new code resolves againsturl.origin(scheme+host only). For root-relative paths like/images/foo.pngthis is equivalent, but the origin check insidevalidateImageUrlresolves againsturl.origintoo (line 190), which means the defense-in-depth check is comparingurl.origin === url.origin— it's a tautology. This was also a tautology in the original code (since/images/foo.pngresolved against the same origin will always keep the same origin), so the security posture is unchanged, but it would be good to document why this defense-in-depth check exists despite being tautological for the filtered inputs. -
Extra blank lines in generated code — The removal of the inline functions left orphaned blank lines in the generated entry at two locations (lines 1049-1051 and 1173-1176). Minor, but noticeable in the generated output.
-
safeRegExpremoved from imports but was previously used inindex.ts— The diff removessafeRegExpfrom theconfig-matchers.jsimport inindex.ts(line 29-38). Verify this isn't used elsewhere in the file or re-exported for consumers.
Correctness Observations
- The
validateCsrfOriginsignature change from closure-captured__allowedOriginsto an explicit parameter is clean and correct. The call site at line 1352 passes__allowedOriginsexplicitly. - The
matchRedirect/matchRewrite/matchHeadersfunctions inconfig-matchers.tstake individual rule arrays, which matches how they're called in the generated code (e.g.,matchRedirect(__redirPathname, __configRedirects, __reqCtx)). - The
requestContextFromRequestreplacement for__buildRequestContextis a straightforward 1:1 swap with the same semantics. - The
matchConfigPatternre-export fromindex.ts(line 3749) preserves the public API for external consumers. - Test updates correctly reflect the new function names and are otherwise semantically equivalent.
Suggestions
See inline comments.
| return new Response("404 Not Found", { status: 404 }); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
guardProtocolRelativeUrl is exported but never imported anywhere. The protocol-relative URL guard is still inlined in app-dev-server.ts at line 1125:
if (url.pathname.replaceAll("\\\\" , "/").startsWith("//")) {Either replace the inline check with this function, or remove it from this PR. Shipping dead code in a module intended to deduplicate code is counterproductive.
| return pathname.slice(basePath.length) || "/"; | ||
| } | ||
| return pathname; | ||
| } |
There was a problem hiding this comment.
stripBasePath is exported but never imported anywhere in the codebase. The inline pathname.slice(basePath.length) || "/" pattern is still used directly in app-dev-server.ts (line 1141) and index.ts. Same concern as guardProtocolRelativeUrl — either wire it up or defer to a follow-up PR.
| }); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
normalizeTrailingSlash is exported but never called. The trailing slash logic is still inlined in app-dev-server.ts at lines 1146-1153. Same issue.
| for (const key of keysToDelete) { | ||
| headers.delete(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
processMiddlewareHeaders is exported but never imported. The middleware header unpacking is still done inline in app-dev-server.ts at lines 1260-1267. This is the fourth unused function in this module.
To summarize: only validateCsrfOrigin and validateImageUrl from this file are actually wired up. The other four functions are dead code.
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Stray extra blank lines left behind from removing the inline functions. Two consecutive blank lines here; please collapse to one.
| }); | ||
| return new Response(upstream.body, { status: upstream.status, statusText: upstream.statusText, headers: respHeaders }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Same issue — two consecutive blank lines left over from the removal of __proxyExternalRequest and __applyConfigHeaders. Please collapse.
| // Validate the constructed URL's origin hasn't changed (defense in depth). | ||
| const url = new URL(requestUrl); | ||
| const resolvedImg = new URL(imgUrl, url.origin); | ||
| if (resolvedImg.origin !== url.origin) { | ||
| return new Response("Only relative URLs allowed", { status: 400 }); | ||
| } | ||
| return imgUrl; |
There was a problem hiding this comment.
The defense-in-depth origin check is tautological for all inputs that pass the guard above (must start with / but not //). Resolving a root-relative path against url.origin will always produce the same origin. This was also true in the original inline code, but since this is now a standalone function with its own documentation, it would be helpful to add a comment explaining that this check exists as a safety net against future changes to the guard logic, not because it catches anything today.
|
Review submitted with REQUEST_CHANGES status. Here's a summary of the key findings: The PR's direction is sound — deduplicating ~250 lines of inline Issues flagged:
Correctness is solid for the parts that are actually wired up: |
yunus25jmi1
force-pushed
the
refactor/shared-request-handling
branch
from
6624adc to
439ebd3
Compare
|
@james-elicx Re-review the changes. Kindly merge the PR. |
|
/bigbonk re-review this pr |
There was a problem hiding this comment.
Review: Extract shared request handling from server entry points
The direction is right — deduplicating inline __-prefixed helpers into importable modules is a clear improvement, and replacing the chained .replace() matchConfigPattern with the canonical single-pass tokenizer from config-matchers.ts is a real security fix. The safeRegExp removal is clean, the matchConfigPattern re-export preserves the public API correctly, and the test updates are mechanically sound.
However, there are correctness issues and misleading documentation that need to be addressed before this can merge.
Issues
1. normalizeTrailingSlash changes redirect behavior from absolute to relative URLs
The old code used Response.redirect(new URL(path, request.url), 308) which produces an absolute Location header. The new code uses new Response(null, { status: 308, headers: { Location: relativePath } }) which produces a relative path. This is not a regression — relative Location headers are valid per RFC 7231, and prod-server.ts already uses relative paths (line 769). So this actually brings dev/prod into alignment. However, it's a deliberate behavioral change that should be called out in the PR description rather than hidden.
2. processMiddlewareHeaders does more than the code it replaced
The old inline code only deleted x-middleware-* headers. The new processMiddlewareHeaders also has logic to unpack x-middleware-request-* into an applyTo target. At the call site (line 1245), applyTo is not passed, so the extra logic is dead. This isn't a bug, but the function's documented behavior ("copies x-middleware-request-* values to actual header names") is misleading at this call site since applyMiddlewareRequestHeaders() on line 1244 already handles that unpacking. The applyTo parameter suggests this function should be a complete replacement for applyMiddlewareRequestHeaders + the delete loop, but it isn't actually used that way.
3. Module header comment is inaccurate
The request-pipeline.ts module comment (lines 4-6) claims these utilities were "extracted from the four server entry points (app-dev-server.ts, dev-server.ts, prod-server.ts, index.ts configureServer)." In reality, only app-dev-server.ts uses them. dev-server.ts, prod-server.ts, and index.ts configureServer still have their own inline implementations. This should either say "extracted from app-dev-server.ts" or the other files should be updated to use these shared functions too (which is presumably the intent for a follow-up).
4. No unit tests for the new request-pipeline.ts functions
The existing tests only verify that the generated code string contains the right function names. There are no unit tests for the actual runtime behavior of guardProtocolRelativeUrl, stripBasePath, normalizeTrailingSlash, validateCsrfOrigin, validateImageUrl, or processMiddlewareHeaders. Since these functions are now standalone importable modules (not inlined in generated code), they should have unit tests that verify their behavior directly — especially the security-sensitive ones like CSRF validation and image URL validation.
| * | ||
| * Extracted from the four server entry points (app-dev-server.ts, | ||
| * dev-server.ts, prod-server.ts, index.ts configureServer) to ensure | ||
| * consistent behavior across dev and production, Pages Router and App Router. |
There was a problem hiding this comment.
This comment is inaccurate. Only app-dev-server.ts imports from this module. dev-server.ts, prod-server.ts, and index.ts configureServer still have their own inline implementations of these functions. Either update the comment to reflect reality ("extracted from app-dev-server.ts") or wire up the other entry points in this PR.
| * consistent behavior across dev and production, Pages Router and App Router. | |
| * Extracted from the App Router RSC entry (app-dev-server.ts) to enable | |
| * reuse across entry points. Currently consumed by app-dev-server.ts; | |
| * dev-server.ts, prod-server.ts, and index.ts still have inline versions | |
| * that should be migrated in follow-up work. |
| if (trailingSlash && !hasTrailing && !pathname.endsWith(".rsc")) { | ||
| return new Response(null, { | ||
| status: 308, | ||
| headers: { Location: basePath + pathname + "/" + search }, |
There was a problem hiding this comment.
This is a subtle behavioral change from the old code. The old inline code used Response.redirect(new URL(basePath + pathname + "/" + search, request.url), 308) which produces an absolute Location URL. This new code produces a relative path.
This is actually an improvement — it aligns dev behavior with prod-server.ts (line 769) which already uses relative Location headers, and relative redirects are valid per RFC 7231. But it should be documented in the PR description since it changes what browsers see in the Location header.
| * @param headers - The Headers object to modify in place | ||
| * @param applyTo - Optional target Headers to receive the unpacked values | ||
| */ | ||
| export function processMiddlewareHeaders( |
There was a problem hiding this comment.
The applyTo parameter is never used at the call site (line 1245 of app-dev-server.ts). The function's JSDoc says it "copies x-middleware-request-* values to the actual header names" but at the call site, applyMiddlewareRequestHeaders() (called on line 1244) already handles that unpacking. So this function only acts as a delete loop.
This makes the function signature misleading — it looks like it should be a complete replacement for the two-step applyMiddlewareRequestHeaders + delete pattern, but it isn't used that way. Either:
- Remove the
applyToparameter and simplify to just the delete logic (matching what the call site needs), or - Actually use this as the complete replacement (removing the
applyMiddlewareRequestHeaderscall at the call site)
Option 1 is safer for this PR.
| }); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This function doesn't have access to request.url (it only receives pathname, basePath, trailingSlash, search), so it can't construct absolute redirect URLs. This is fine — relative Location is correct — but it means callers can't opt into absolute URLs if they need to. Worth noting in the JSDoc that the returned Response uses a relative Location header.
| ${middlewarePath ? `import * as middlewareModule from ${JSON.stringify(middlewarePath.replace(/\\/g, "/"))};` : ""} | ||
| ${instrumentationPath ? `import * as _instrumentation from ${JSON.stringify(instrumentationPath.replace(/\\/g, "/"))};` : ""} | ||
| ${effectiveMetaRoutes.length > 0 ? `import { sitemapToXml, robotsToText, manifestToJson } from ${JSON.stringify(fileURLToPath(new URL("./metadata-routes.js", import.meta.url)).replace(/\\/g, "/"))};` : ""} | ||
| import { requestContextFromRequest, matchRedirect, matchRewrite, matchHeaders, isExternalUrl, proxyExternalRequest, sanitizeDestination } from ${JSON.stringify(configMatchersPath)}; |
There was a problem hiding this comment.
This import pulls in guardProtocolRelativeUrl, stripBasePath, normalizeTrailingSlash, and processMiddlewareHeaders from request-pipeline.ts. The previous review flagged these as dead code, but they're actually used — they appear in the generated code template below (lines 1117, 1131, 1136, 1245). The confusion arose because they're inside a template literal, not a regular import.
That said, stripBasePath is only emitted when basePath is configured (gated by ${bp ? ...}), and processMiddlewareHeaders is only emitted when middleware exists. The import is unconditional though, which means these functions are imported even when not needed. This is a minor bundle concern for dev-only code — not blocking, but worth noting.
|
Review submitted successfully. Here's a summary of what I flagged: Review SummaryOverall assessment: REQUEST_CHANGES — The deduplication direction is sound, but there are documentation accuracy issues and missing test coverage that should be addressed. Key findings:
|
…quest-pipeline Deduplicate ~250 lines of inline request-handling logic from the generated App Router RSC entry (app-dev-server.ts) and Pages Router server entry (index.ts) by importing from the canonical config-matchers.ts and a new request-pipeline.ts module. Changes: - Create server/request-pipeline.ts with shared utilities: guardProtocolRelativeUrl, stripBasePath, normalizeTrailingSlash, validateCsrfOrigin, isOriginAllowed, validateImageUrl, processMiddlewareHeaders - app-dev-server.ts: replace 14 inline __-prefixed functions with imports from config-matchers.ts (matchRedirect, matchRewrite, matchHeaders, isExternalUrl, proxyExternalRequest, sanitizeDestination, requestContextFromRequest) and request-pipeline.ts (validateCsrfOrigin, validateImageUrl) - index.ts: replace inline parseCookies, sanitizeDestinationLocal, and matchConfigPattern with imports/re-exports from config-matchers.ts; replace manual RequestContext construction with requestContextFromRequest() - Update tests to reference imported function names instead of old inline __-prefixed names Security fixes: - matchConfigPattern: eliminates chained .replace() divergence (CodeQL flagged) in favor of config-matchers single-pass tokenizer - proxyExternalRequest: uses AbortController+setTimeout (broader runtime support) instead of AbortSignal.timeout() - escapeHeaderSource: uses proper config-matchers implementation instead of simplified chained .replace() Closes cloudflare#254 t status
yunus25jmi1
force-pushed
the
refactor/shared-request-handling
branch
from
439ebd3 to
b59724d
Compare
|
/bigbonk review this pr please |
There was a problem hiding this comment.
Review: Extract shared request handling from server entry points
The direction of this PR is right — deduplicating the inline __-prefixed helpers into importable modules is a clear improvement. Replacing the chained .replace() matchConfigPattern with the canonical single-pass tokenizer from config-matchers.ts is a real fix. The module header comment accurately reflects the current state (after the second reviewer's suggestion was adopted). The new request-pipeline.test.ts test file is a meaningful addition.
However, there are a few issues that should be addressed.
Key Issues
-
processMiddlewareHeadershas a misleadingapplyToparameter that is dead at its only call site — The function documents unpackingx-middleware-request-*headers into a target, but the call site (line 1245) never passesapplyTo. The JSDoc says it "copies x-middleware-request-* values to actual header names" butapplyMiddlewareRequestHeaders()on line 1244 already handles that. The tests don't cover theapplyTopath either. This parameter should be removed to match actual usage. -
Dangling comment in
index.ts— Line 3751 (// sanitizeDestination is imported from config-matchers.ts) is a stub comment that doesn't correspond to any code. It's not a re-export, not an import — just a leftover from removingsanitizeDestinationLocal. Should be removed. -
normalizeTrailingSlashchanges redirect behavior — The old code usedResponse.redirect(new URL(..., request.url))producing an absoluteLocationheader. The new code returnsnew Response(null, { status: 308, headers: { Location: relativePath } })producing a relative path. This is actually an improvement (aligns dev withprod-server.ts, and relativeLocationis valid per RFC 7231), but it's a deliberate behavioral change that should be noted in the PR description.
Minor
- The
processMiddlewareHeaderstests only cover the no-applyTocode path. IfapplyTois kept, it needs test coverage.
What looks good
- The
matchConfigPatternre-export fromindex.tspreserves the public API correctly sanitizeDestinationLocal→sanitizeDestinationswap is clean and correctrequestContextFromRequestusage in bothindex.tsandapp-dev-server.tsis a straightforward 1:1 replacementvalidateCsrfOriginsignature change from closure-captured__allowedOriginsto an explicit parameter is clean- The test file exercises all 6 exported functions with reasonable coverage
- The module header comment is now accurate (after incorporating the second bot review's suggestion)
| * @param headers - The Headers object to modify in place | ||
| * @param applyTo - Optional target Headers to receive the unpacked values | ||
| */ | ||
| export function processMiddlewareHeaders( |
There was a problem hiding this comment.
The applyTo parameter is never passed at the only call site (app-dev-server.ts:1245). The function's JSDoc says it "copies x-middleware-request-* values to the actual header names" but this step is already handled by applyMiddlewareRequestHeaders() (called on line 1244, imported from next/headers). So at the call site, this function only acts as a delete loop.
The applyTo parameter and its associated logic (lines 224-225) are dead code. The test file also doesn't cover this path.
Suggestion: remove applyTo and simplify. If you intend this function to eventually replace applyMiddlewareRequestHeaders entirely, that should happen in this PR (replacing the two-step pattern) rather than shipping dead code.
| export function processMiddlewareHeaders( | |
| export function processMiddlewareHeaders( | |
| headers: Headers, | |
| ): void { |
| const prefix = "x-middleware-request-"; | ||
| const keysToDelete: string[] = []; | ||
|
|
||
| for (const key of headers.keys()) { | ||
| if (key.startsWith(prefix)) { | ||
| const realName = key.slice(prefix.length); | ||
| if (applyTo) { | ||
| applyTo.set(realName, headers.get(key)!); | ||
| } |
There was a problem hiding this comment.
If applyTo is removed per the comment above, this block simplifies to just collecting keys to delete:
| const prefix = "x-middleware-request-"; | |
| const keysToDelete: string[] = []; | |
| for (const key of headers.keys()) { | |
| if (key.startsWith(prefix)) { | |
| const realName = key.slice(prefix.length); | |
| if (applyTo) { | |
| applyTo.set(realName, headers.get(key)!); | |
| } | |
| const keysToDelete: string[] = []; | |
| for (const key of headers.keys()) { | |
| if (key.startsWith("x-middleware-")) { | |
| keysToDelete.push(key); | |
| } | |
| } |
packages/vinext/src/index.ts
Outdated
| // divergence that CodeQL flagged as incomplete sanitization). | ||
| export { matchConfigPattern } from "./config/config-matchers.js"; | ||
|
|
||
| // sanitizeDestination is imported from config-matchers.ts |
There was a problem hiding this comment.
This comment is a dangling stub — it doesn't correspond to any code. sanitizeDestination is imported at line 36 and used in applyRedirects/applyRewrites below. The comment was presumably left behind when sanitizeDestinationLocal was removed. Remove it.
| // sanitizeDestination is imported from config-matchers.ts |
| if (trailingSlash && !hasTrailing && !pathname.endsWith(".rsc")) { | ||
| return new Response(null, { | ||
| status: 308, | ||
| headers: { Location: basePath + pathname + "/" + search }, |
There was a problem hiding this comment.
This is a subtle behavioral change from the old code. The old inline code used Response.redirect(new URL(basePath + pathname + "/" + search, request.url), 308) which produces an absolute Location URL. This new code produces a relative path.
This is actually an improvement — it aligns dev behavior with prod-server.ts (which already uses relative Location headers), and relative redirects are valid per RFC 7231. But it's a behavioral change that should be documented in the PR description since it changes what browsers see in the Location header.
| expect(headers.get("content-type")).toBe("text/html"); | ||
| expect(headers.get("x-custom")).toBe("keep"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for the applyTo parameter of processMiddlewareHeaders. If that parameter is kept (see other comment), there should be a test like:
it("unpacks x-middleware-request-* headers into applyTo target", () => {
const headers = new Headers({
"x-middleware-request-x-custom": "value",
"x-middleware-next": "1",
});
const target = new Headers();
processMiddlewareHeaders(headers, target);
expect(target.get("x-custom")).toBe("value");
expect(headers.has("x-middleware-request-x-custom")).toBe(false);
expect(headers.has("x-middleware-next")).toBe(false);
});Or remove the parameter entirely (preferred — see review comment on request-pipeline.ts:214).
|
Review submitted successfully. Here's a summary of what I flagged: REQUEST_CHANGES with 5 inline comments:
The overall deduplication direction is sound — the |
…uest-handling # Conflicts: # packages/vinext/src/index.ts
|
/bigbonk review this pr please |
There was a problem hiding this comment.
Review: Extract shared request handling from server entry points
The direction is right — consolidating inline __-prefixed helpers into importable modules is a clear improvement, and replacing the chained .replace() matchConfigPattern with the canonical single-pass tokenizer from config-matchers.ts is a real security fix. The new request-pipeline.test.ts file adds meaningful coverage for the extracted functions. Tests pass (33/33 request-pipeline, 209/209 app-router).
I've verified that all imported functions (requestContextFromRequest, matchRedirect, matchRewrite, matchHeaders, sanitizeDestination, isExternalUrl, proxyExternalRequest, parseCookies) exist in config-matchers.ts with compatible signatures, and the safeRegExp removal is clean (no remaining references in index.ts).
Two items that should be addressed:
1. Dangling comment on line 3856 of index.ts
The comment // sanitizeDestination is imported from config-matchers.ts sits alone between the matchConfigPattern re-export and the stripServerExports function. It's a leftover from removing sanitizeDestinationLocal and doesn't correspond to any code — sanitizeDestination is imported at line 37 and used in applyRedirects/applyRewrites further down. This should be removed.
2. Orphaned comment on line 1094 of index.ts (generated code)
The comment // parseCookies is imported from config-matchers.ts (see import statements above) replaces the old inline parseCookies function in the generated entry template. While it's not harmful, leaving a comment in generated runtime code pointing at import statements (that the user can't see in the generated output) is confusing. Either remove the comment entirely or replace it with something minimal like // (imported above).
Minor observations (non-blocking)
-
normalizeTrailingSlashuses relativeLocationheaders where the old code usedResponse.redirect(new URL(..., request.url))producing absolute URLs. This is actually an improvement (aligns dev withprod-server.ts, and relativeLocationis valid per RFC 7231), but it's a behavioral change worth noting in the PR description. -
processMiddlewareHeadersis clean and correct — the previous bot reviews flagged a now-removedapplyToparameter that no longer exists in the code. The current implementation is just a delete loop, which is exactly what the call site needs. -
Import path conventions are consistent:
app-dev-server.tsusesfileURLToPath(new URL(..., import.meta.url))(ESM style) whileindex.tsusespath.resolve(__dirname, ...)(CJS-compat style). Both are established patterns in their respective files.
packages/vinext/src/index.ts
Outdated
| // divergence that CodeQL flagged as incomplete sanitization). | ||
| export { matchConfigPattern } from "./config/config-matchers.js"; | ||
|
|
||
| // sanitizeDestination is imported from config-matchers.ts |
There was a problem hiding this comment.
This comment is a dangling stub — it doesn't correspond to any code. sanitizeDestination is imported at line 37 and used in applyRedirects/applyRewrites below. This was left behind when sanitizeDestinationLocal was removed. Remove it.
| // sanitizeDestination is imported from config-matchers.ts |
packages/vinext/src/index.ts
Outdated
| } | ||
| return cookies; | ||
| } | ||
| // parseCookies is imported from config-matchers.ts (see import statements above) |
There was a problem hiding this comment.
This comment in the generated entry template is confusing — it points at "import statements above" that aren't visible in the generated output at runtime. Consider removing it or making it minimal:
| // parseCookies is imported from config-matchers.ts (see import statements above) |
|
Review submitted successfully. Here's a summary of what I found: Overall: The PR is solid. The deduplication of ~250 lines of inline Two items to address:
Non-blocking observations:
|
|
/bigbonk please review this pr. |
There was a problem hiding this comment.
Review: Extract shared request handling from server entry points
The direction of this PR is sound. Extracting inline __-prefixed helpers into importable modules is a clear structural improvement, and replacing the chained .replace() matchConfigPattern with the canonical single-pass tokenizer from config-matchers.ts is a real security fix. The new request-pipeline.ts module is well-documented, the function signatures are clean, and the test file adds meaningful coverage for all six exported functions.
What looks good
matchConfigPatternre-export fromindex.tspreserves the public API correctly (line 3852) with a clear comment explaining why.sanitizeDestinationLocal→sanitizeDestinationswap is clean — the config-matchers version has the same semantics.requestContextFromRequestis a straightforward 1:1 replacement for__buildRequestContextat both call sites (app-dev-server.ts:1197, index.ts:2995).validateCsrfOriginsignature change from closure-captured__allowedOriginsto an explicit parameter is correct and improves testability.safeRegExpremoval from theindex.tsimport is verified clean — zero remaining references.processMiddlewareHeadersis simple and correct — just a delete loop, which is exactly what the call site needs (line 1374, afterapplyMiddlewareRequestHeadersalready handles unpacking).- Module header comment (lines 4-7) is accurate after incorporating the earlier review feedback.
- Test file exercises all six exported functions with good edge case coverage.
Issues to address
See inline comments — two minor items that should be cleaned up.
| if (trailingSlash && !hasTrailing && !pathname.endsWith(".rsc")) { | ||
| return new Response(null, { | ||
| status: 308, | ||
| headers: { Location: basePath + pathname + "/" + search }, |
There was a problem hiding this comment.
Non-blocking observation: This is a subtle behavioral change from the old code. The old inline version used Response.redirect(new URL(basePath + pathname + "/" + search, request.url), 308), which produces an absolute Location header. This new version produces a relative path.
This is actually an improvement — it aligns dev behavior with prod-server.ts (which already uses relative Location headers), and relative redirects are valid per RFC 7231 §7.1.2. But since it changes what browsers see in the Location header, it's worth noting in the PR description as a deliberate behavioral change.
| if (__imgResult instanceof Response) return __imgResult; | ||
| // In dev, redirect to the original asset URL so Vite's static serving handles it. | ||
| return Response.redirect(__resolvedImg.href, 302); | ||
| return Response.redirect(new URL(__imgResult, url.origin).href, 302); |
There was a problem hiding this comment.
Good catch on simplifying the image validation flow. One thing worth noting: the old code resolved against request.url (full URL with path+query) while the new validateImageUrl resolves against url.origin. For root-relative paths this is equivalent. The defense-in-depth origin check inside validateImageUrl is documented as tautological for current inputs (line 191-194 of request-pipeline.ts), which is the right call.
| ${middlewarePath ? `import * as middlewareModule from ${JSON.stringify(middlewarePath.replace(/\\/g, "/"))};` : ""} | ||
| ${instrumentationPath ? `import * as _instrumentation from ${JSON.stringify(instrumentationPath.replace(/\\/g, "/"))};` : ""} | ||
| ${effectiveMetaRoutes.length > 0 ? `import { sitemapToXml, robotsToText, manifestToJson } from ${JSON.stringify(fileURLToPath(new URL("./metadata-routes.js", import.meta.url)).replace(/\\/g, "/"))};` : ""} | ||
| import { requestContextFromRequest, matchRedirect, matchRewrite, matchHeaders, isExternalUrl, proxyExternalRequest, sanitizeDestination } from ${JSON.stringify(configMatchersPath)}; |
There was a problem hiding this comment.
Minor: Both imports are unconditional, but some of the imported functions are only emitted inside conditional template blocks — stripBasePath is gated by ${bp ? ...}, processMiddlewareHeaders is gated by middleware existence, etc. Since these imports appear in the generated virtual RSC entry (not the build output), the tree-shaking concern is negligible for dev-only code. Just noting this is intentional/acceptable.
| host: request.headers.get("host") || url.host, | ||
| }; | ||
| } | ||
| // ── Config pattern matching, redirects, rewrites, headers, CSRF validation, |
There was a problem hiding this comment.
This is a nice replacement for the ~250 lines of inline code. The comment is clear and explains the rationale well. The generateSafeRegExpCode and generateNormalizePathCode remaining inline (lines 1078, 1081) makes sense since they generate function definitions that middleware matching needs at runtime in the virtual entry.
|
Review submitted successfully. Here's a summary of what I found: Overall assessment: COMMENT (no blocking issues) The PR is in good shape. The deduplication of ~250 lines of inline Key findings:
|
|
All review items addressed across 3 commits:
The PR description has also been updated to document the Latest Bonk review confirmed no blocking issues. |
|
@james-elicx review the PR. |
|
/bigbonk please re-review the pr |
There was a problem hiding this comment.
Review: Extract shared request handling from server entry points
This is a well-structured refactoring PR. The deduplication of inline __-prefixed helpers into importable modules is a clear improvement, and replacing the chained .replace() matchConfigPattern with the canonical single-pass tokenizer from config-matchers.ts is a real security fix. The code quality, documentation, and test coverage are solid.
What looks good
matchConfigPatternconsolidation — Removing the duplicate implementation fromindex.ts(~120 lines) and re-exporting fromconfig-matchers.tsis the highest-value change. The single-pass tokenizer is correct and eliminates the CodeQL-flagged chained.replace()divergence.requestContextFromRequest— Clean 1:1 replacement for__buildRequestContextat both call sites. The Node.jsIncomingMessage→ WebRequestconversion inindex.ts:2985-2996is correctly documented.validateCsrfOriginparameterization — Moving from closure-captured__allowedOriginsto an explicit parameter improves testability and makes the dependency explicit.processMiddlewareHeadersis clean — a simple delete loop matching what the call site needs.- Module header comment (lines 4-7 of
request-pipeline.ts) is accurate and honest about the migration scope. - Test coverage — 33 tests covering all 6 exported functions with good edge case coverage (backslash variants, wildcard subdomains, missing parameters, etc.)
safeRegExpremoval fromindex.tsimports is verified clean — zero remaining references.
Issues
See inline comments for two items worth addressing.
| { | ||
| const __tsRedirect = normalizeTrailingSlash(pathname, __basePath, __trailingSlash, url.search); |
There was a problem hiding this comment.
The old inline code conditionally ran trailing-slash logic:
if (pathname !== "/" && !pathname.startsWith("/api")) {The new code unconditionally calls normalizeTrailingSlash() which has the same guard internally, so correctness is preserved. However, wrapping the call in a bare block { ... } to scope __tsRedirect is an unusual pattern in generated code. A simpler approach:
| { | |
| const __tsRedirect = normalizeTrailingSlash(pathname, __basePath, __trailingSlash, url.search); | |
| const __tsRedirect = normalizeTrailingSlash(pathname, __basePath, __trailingSlash, url.search); | |
| if (__tsRedirect) return __tsRedirect; |
The __tsRedirect variable doesn't need scoping — it's immediately checked and the function returns if non-null. The block is harmless but adds visual noise to the generated entry.
| if (__basePath && pathname.startsWith(__basePath)) { | ||
| pathname = pathname.slice(__basePath.length) || "/"; | ||
| } | ||
| pathname = stripBasePath(pathname, __basePath); |
There was a problem hiding this comment.
Minor: when bp is truthy, this emits pathname = stripBasePath(pathname, __basePath). The stripBasePath function already handles the case where basePath is empty or the pathname doesn't start with basePath, so the conditional template guard ${bp ? ... : ""} is technically redundant — you could always emit the call. Not suggesting a change here since the conditional avoids a no-op function call in the common case (no basePath), just noting the safety margin.
| if (trailingSlash && !hasTrailing && !pathname.endsWith(".rsc")) { | ||
| return new Response(null, { | ||
| status: 308, | ||
| headers: { Location: basePath + pathname + "/" + search }, |
There was a problem hiding this comment.
This is the behavioral change documented in the PR description — switching from Response.redirect(new URL(..., request.url)) (absolute Location) to a relative Location header. I've verified that prod-server.ts (lines 769, 774) already uses relative Location headers with the same pattern:
res.writeHead(308, { Location: basePath + pathname + "/" + qs });So this actually brings dev/prod into alignment. The change is correct and the PR description properly documents it.
| import { safeJsonStringify } from "vinext/html"; | ||
| import { getSSRFontLinks as _getSSRFontLinks, getSSRFontStyles as _getSSRFontStylesGoogle, getSSRFontPreloads as _getSSRFontPreloadsGoogle } from "next/font/google"; | ||
| import { getSSRFontStyles as _getSSRFontStylesLocal, getSSRFontPreloads as _getSSRFontPreloadsLocal } from "next/font/local"; | ||
| import { parseCookies } from ${JSON.stringify(path.resolve(__dirname, "config/config-matchers.js").replace(/\\/g, "/"))}; |
There was a problem hiding this comment.
Good change. The generated Pages Router entry now imports parseCookies from the canonical config-matchers.ts instead of inlining its own copy. The import uses path.resolve(__dirname, ...) which is consistent with the established CJS-compat pattern in this file (vs. the fileURLToPath(new URL(..., import.meta.url)) ESM pattern in app-dev-server.ts).
| it("returns null when pathname already matches the trailingSlash setting", () => { | ||
| expect(normalizeTrailingSlash("/about/", "", true, "")).toBeNull(); | ||
| expect(normalizeTrailingSlash("/about", "", false, "")).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
The trailing slash tests are thorough but missing one edge case: multiple trailing slashes. The regex pathname.replace(/\/+$/, "") in normalizeTrailingSlash handles /about/// → /about, but there's no test for it. Consider adding:
it("strips multiple trailing slashes when trailingSlash is false", () => {
const res = normalizeTrailingSlash("/about///", "", false, "");
expect(res).not.toBeNull();
expect(res!.headers.get("Location")).toBe("/about");
});| }); | ||
| // When host is missing, the function returns null (allows) | ||
| expect(validateCsrfOrigin(req)).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
Good coverage of the CSRF validation. One additional edge case worth considering: the Host header with a port vs the Origin without a port (or vice versa). For example:
Host: localhost:3000+Origin: http://localhost(origin omits port, but port 80/443 are implicit)- This is correctly handled by
new URL(originHeader).hostwhich includes the port when non-default.
Not blocking — just noting the test doesn't exercise port mismatch scenarios, which are covered by the host property behavior of the URL constructor.
|
Review submitted successfully. Here's a summary of my findings: Overall: This is a solid refactoring PR. The direction is right — deduplicating ~250 lines of inline Key verified items:
Inline feedback posted:
|
|
Addressed the two actionable items from the latest bonk review:
All 243 tests pass (209 app-router + 34 request-pipeline). |
|
Review the PR and merge it. @james-elicx |
|
/bigbonk re-review the pr please |
There was a problem hiding this comment.
Review
Solid refactoring. The core value here is real: consolidating the chained .replace() matchConfigPattern onto the canonical single-pass tokenizer from config-matchers.ts, and replacing the ~250 lines of inline __-prefixed helpers with imports. The new request-pipeline.ts module is well-documented, the function signatures are clean, and the test coverage (34 tests) is meaningful.
I verified:
- All imports from
config-matchers.tsandrequest-pipeline.tshave matching exports with compatible signatures safeRegExpremoval fromindex.tsimports is clean (zero remaining references)parseCookiesin the generated Pages Router entry correctly imports from the canonicalconfig-matchers.tsand the call site at line 1106 matches the old inline usage- The
normalizeTrailingSlashbehavioral change (absolute → relativeLocation) aligns dev withprod-server.ts(lines 769, 774 use relative paths) - Previous reviewers' dangling comment concerns have been addressed (no stale
sanitizeDestinationorparseCookiescomments remain) processMiddlewareHeadersis correctly simplified to a delete loop matching the call site need
Two minor items inline — neither is blocking.
| export function guardProtocolRelativeUrl(rawPathname: string): Response | null { | ||
| // Normalize backslashes: browsers and the URL constructor treat | ||
| // /\evil.com as protocol-relative (//evil.com), bypassing the // check. | ||
| if (rawPathname.replaceAll("\\", "/").startsWith("//")) { |
There was a problem hiding this comment.
Nit: String.prototype.replaceAll is available in Node 15+ and all modern browsers, so this is fine for runtime. But the old inline code in the generated entry used double-backslash "\\\\" (escaped for the template literal context), while this version uses a single "\\". Both are correct — the old code was inside a template literal where \\ becomes \ at runtime, and this code is a regular .ts file where "\\" is already \. Just noting for anyone confused reading the diff.
| trailingSlash: boolean, | ||
| search: string, | ||
| ): Response | null { | ||
| if (pathname === "/" || pathname.startsWith("/api")) { |
There was a problem hiding this comment.
Minor edge case worth considering: paths like /api-docs/ will not be redirected because pathname.startsWith("/api") matches them. This matches the old inline behavior exactly (same guard), so it's not a regression. But Next.js actually checks for /api/ or exactly /api — the prefix match here is slightly too broad.
Not something to fix in this PR since it's a pre-existing behavioral difference, but worth filing as a follow-up issue.
|
Review submitted. Here's a summary: Overall assessment: COMMENT (no blocking issues) The PR is in good shape. The core improvements are:
I posted two non-blocking inline comments:
|
|
Latest Bonk review confirmed no blocking issues — both inline comments are non-blocking observations:
The stale @james-elicx could you dismiss it? |
|
i was going to reply to bonk's comment on /api that it would probably be worth addressing in this one. i can take care of it in the morning if you want |
…ches only /api or /api/), add tests for edge cases
|
Addressed the /api prefix edge case:
Ready for merge. Please dismiss stale REQUEST_CHANGES. |
2 participants
Summary
Deduplicates ~250 lines of inline request-handling logic from the generated App Router RSC entry (
app-dev-server.ts) and Pages Router server entry (index.ts) by importing from the canonicalconfig-matchers.tsand a newrequest-pipeline.tsmodule.Changes
New file:
server/request-pipeline.tsShared request lifecycle utilities extracted for reuse across entry points:
guardProtocolRelativeUrl()— blocks//evil.comprotocol-relative URL attacksstripBasePath()— removes configured basePath prefix from pathnamesnormalizeTrailingSlash()— redirects to canonical trailing-slash formvalidateCsrfOrigin()— CSRF origin validation for server actionsisOriginAllowed()— wildcard subdomain matching for allowed originsvalidateImageUrl()— image optimization URL validationprocessMiddlewareHeaders()— strips internalx-middleware-*headersapp-dev-server.ts(~250 lines removed)Replaced 14 inline
__-prefixed functions with imports fromconfig-matchers.tsandrequest-pipeline.ts:matchRedirect,matchRewrite,matchHeaders,sanitizeDestinationisExternalUrl,proxyExternalRequestrequestContextFromRequestvalidateCsrfOrigin,validateImageUrlindex.tsparseCookiesin generated server entry template with import fromconfig-matchers.tsRequestContextconstruction withrequestContextFromRequest()sanitizeDestinationLocal(), using importedsanitizeDestination()matchConfigPattern+extractConstraint(~100 lines), re-exporting fromconfig-matchers.tsTest updates
app-router.test.tsassertions to reference the new imported function names instead of old inline__-prefixed names.request-pipeline.test.tswith 33 unit tests covering all 6 exported functions.Behavioral Change
normalizeTrailingSlashnow uses relativeLocationheaders instead of absolute URLs. The old inline code usedResponse.redirect(new URL(..., request.url), 308)which produces an absoluteLocationheader; the new code usesnew Response(null, { status: 308, headers: { Location: relativePath } }). This is valid per RFC 7231 §7.1.2 and aligns dev behavior withprod-server.ts(which already uses relativeLocationheaders).Security Fixes
.replace()divergence (CodeQL flagged incomplete sanitization) in favor of config-matchers single-pass tokenizerAbortController+setTimeout(broader runtime support) instead ofAbortSignal.timeout().replace()Verification
pnpm run typecheckpasses cleanCloses #254